HTTP proxy support reworked #751
Conversation
This is missing proper error handling and tests.
andrewshadura
force-pushed
the
http-proxy-support-reworked
branch
from
b8ad1db to
4bc7408
Compare
|
Oh well, last minute fixes without running tests — I should have not done that 🙂 |
andrewshadura
force-pushed
the
http-proxy-support-reworked
branch
from
4bc7408 to
16a1cd3
Compare
|
Preliminary note -- Python 3.6 is officially supported until 2021. It feels a bit early to drop support for this version. Perhaps we could make HTTP proxy support a Python 3.7+ only feature, while preserving compatibility with Python 3.6 for everything else. |
|
Sure. I can submit a revised pull request. Would you prefer connect() to throw an exception when accessing an https:// URI behind a proxy when start_tls is not available or simply let it fail further down the line while documenting it thoroughly?
|
|
Perhaps it's best if I spend more time reviewing before deciding on the next steps. |
andrewshadura
force-pushed
the
http-proxy-support-reworked
branch
from
16a1cd3 to
65c0980
Compare
|
Great job @andrewshadura and @aaugustin! I'm looking froward to this. Is there anything I can help with? |
aaugustin
force-pushed
the
master
branch
2 times, most recently
from
5246956 to
ecf64e7
Compare
|
This is great. I am wanting to move from autobahn for its lack of proxy w/ asyncio support. |
|
Unfortunately, there were some upstream changes which most likely conflict with my PR, so I guess it will require some significant rework once again. |
|
Support for Python 3.6 was dropped, which lifts the biggest blocker to this. However, in the meantime, the library has undergone a major refactoring. I'm writing a "new" implementation on top of a Sans-I/O core, which will make it vastly easier to implement this feature. I'm increasingly reluctant to add features to what has become the "legacy" implementation because this increases the size of the target. Of course, all this doesn't really help you in the short term :-/ |
|
Thanks @andrewshadura I have changed code in my local repo of version 9.1 and it works for me! There is very little conflicts. |
aaugustin
force-pushed
the
main
branch
2 times, most recently
from
1726dba to
61e0e1c
Compare
|
@xiaocairush are you able to share the changes you made to get it working? |
|
While we have deployed this code for the specific feature the customer has requested, the feature was well-tested and worked, they never enabled it in production, and will unlikely to do so short-term. Meanwhile we pinned Given the amount of refactoring that happened in the project since this was submitted, I think a new PR with this functionality re-implemented once again in case of demand should be more appropriate 🙂 |
Hi,
I needed HTTP support in websockets, so I took an existing PR #422 and ported it to the latest version of the main code. Since
start_tlshas been available for a while now, the hack with upgrading HTTP to HTTPS is no longer needed.My original implementation was a separate module overriding things and providing proxying support on top of the unmodified websockets library. I have tested that one quite a lot and then ported and integrated it back. I have not written any tests since I’m not quite sure how to approach this.
I have changed the implementation of
ProxyURIto allow a trailing slash in the proxy URI. While this is non-standard, it seems to be widely supported and many users are unaware that the trailing slash is not supposed to be used in the proxy URIs.What doesn’t work yet:
hostandportkeyword parameters change the host and port used to connect to the proxy, not the targetproxy_uriparsingThis fixes #364, supersedes and closes #422.